Unsafe evolution: unsafe fields#83694
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the C# compiler’s “unsafe evolution” implementation so that fields can be explicitly marked unsafe under the updated memory safety rules, making field access require an unsafe context via RequiresUnsafeAttribute/CallerUnsafeMode.
Changes:
- Add field support to
RequiresUnsafeAttributeand update compiler tests to validate unsafe field behavior and metadata. - Implement
CallerUnsafeModefor fields across source, metadata, and wrapper/synthesized field symbols (including synthesis and decoding ofRequiresUnsafeAttribute). - Adjust binder behavior so
unsafeon declarations does not introduce an unsafe region under updated rules, and emit diagnostics (not just DEBUG asserts) when inline-array element fields are caller-unsafe.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs | Updates the test definition of RequiresUnsafeAttribute to allow Field targets. |
| src/Compilers/CSharp/Test/CSharp15/UnsafeEvolutionTests.cs | Adds coverage for unsafe fields, including metadata validation for PE fields. |
| src/Compilers/CSharp/Portable/Symbols/Tuples/TupleFieldSymbol.cs | Propagates CallerUnsafeMode from the underlying field for tuple element fields. |
| src/Compilers/CSharp/Portable/Symbols/Tuples/TupleErrorFieldSymbol.cs | Explicitly sets CallerUnsafeMode to None for error tuple fields. |
| src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedLambdaCacheFieldSymbol.cs | Sets synthesized lambda cache fields to CallerUnsafeMode.None. |
| src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedFieldSymbol.cs | Sets synthesized fields to CallerUnsafeMode.None. |
| src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedEnumValueFieldSymbol.cs | Sets enum backing fields to CallerUnsafeMode.None. |
| src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedBackingFieldSymbol.cs | Sets auto-property backing fields to CallerUnsafeMode.None. |
| src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedPrimaryConstructorParameterBackingFieldSymbol.cs | Sets primary-ctor backing fields to CallerUnsafeMode.None. |
| src/Compilers/CSharp/Portable/Symbols/SubstitutedFieldSymbol.cs | Delegates CallerUnsafeMode to the underlying field for substituted fields. |
| src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberFieldSymbol.cs | Implements explicit caller-unsafe fields under updated rules and synthesizes RequiresUnsafeAttribute. |
| src/Compilers/CSharp/Portable/Symbols/Source/SourceEnumConstantSymbol.cs | Ensures enum constants remain CallerUnsafeMode.None. |
| src/Compilers/CSharp/Portable/Symbols/Source/FieldSymbolWithAttributesAndModifiers.cs | Reports ERR_RequiresUnsafeAttributeInSource when RequiresUnsafeAttribute is applied to fields in source. |
| src/Compilers/CSharp/Portable/Symbols/Retargeting/RetargetingFieldSymbol.cs | Delegates CallerUnsafeMode to the underlying field for retargeting wrappers. |
| src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEFieldSymbol.cs | Decodes/filter-caches RequiresUnsafeAttribute from metadata and exposes it via CallerUnsafeMode. |
| src/Compilers/CSharp/Portable/Symbols/FieldSymbol.cs | Removes the previous “always None” default to allow field-specific CallerUnsafeMode. |
| src/Compilers/CSharp/Portable/Symbols/AnonymousTypes/SynthesizedSymbols/AnonymousType.FieldSymbol.cs | Sets anonymous type backing fields to CallerUnsafeMode.None. |
| src/Compilers/CSharp/Portable/Lowering/StateMachineRewriter/StateMachineFieldSymbol.cs | Sets state machine fields to CallerUnsafeMode.None. |
| src/Compilers/CSharp/Portable/Lowering/ClosureConversion/LambdaCapturedVariable.cs | Sets captured-variable fields to CallerUnsafeMode.None. |
| src/Compilers/CSharp/Portable/Binder/Binder_Flags.cs | Updates unsafe-region introduction rules under updated memory safety rules. |
| src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs | Emits diagnostics for unsafe inline-array element field access. |
| src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs | Emits diagnostics for unsafe inline-array conversions via caller-unsafe element fields. |
unsafe fieldsunsafe fields
|
@333fred @AlekseyTs for reviews, thanks |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s), but failed to run 1 pipeline(s). |
|
@333fred @AlekseyTs for reviews, thanks |
333fred
left a comment
There was a problem hiding this comment.
LGTM, assuming tests are passing
|
@AlekseyTs for second review, thanks |
|
|
||
| diagnostics.ReportUseSite(elementField, syntax); | ||
| AssertNotUnsafeMemberAccess(elementField); // https://github.com/dotnet/roslyn/issues/82546: Support unsafe fields? | ||
| ReportDiagnosticsIfUnsafeMemberAccess(diagnostics, elementField, syntax); |
There was a problem hiding this comment.
I think that motivation for reporting use-site was mostly about reporting bad field types/declarations
There was a problem hiding this comment.
I guess, my question is whether it was an explicit design decision to check underlying field for "safety".
There was a problem hiding this comment.
I guess, my question is whether it was an explicit design decision to check underlying field for "safety".
The general rule we established, I think, is to check user-declared members that the compiler uses. But we don't need to check well-known members.
For example, it was discussed in last LDM for unsafe https://github.com/dotnet/csharplang/blob/main/meetings/2026/LDM-2026-05-13.md#implicit-calls-and-constructor-edge-cases:
When compiler-generated or implicit execution would call a caller-unsafe member, we will either require an unsafe context or produce an error; we will not silently allow the call.
Do we actually access the field explicitly here? I don't think we do.
That's a good point, I haven't realized that. I will remove this check and we can add an open question for inline array fields.
| CheckFeatureAvailability(node, MessageID.IDS_FeatureInlineArrays, diagnostics); | ||
| diagnostics.ReportUseSite(elementField, node); | ||
| AssertNotUnsafeMemberAccess(elementField); // https://github.com/dotnet/roslyn/issues/82546: Support unsafe fields? | ||
| ReportDiagnosticsIfUnsafeMemberAccess(diagnostics, elementField, node); |
| targetFramework: TargetFramework.Net100, | ||
| options: TestOptions.UnsafeReleaseDll.WithUpdatedMemorySafetyRules()) | ||
| .VerifyDiagnostics( | ||
| // (7,13): error CS9362: 'Buffer._element0' must be used in an unsafe context because it is marked as 'unsafe' or 'extern' |
|
Done with review pass (commit 5) #Closed |
Test plan: #81207
Relevant speclet section: https://github.com/dotnet/csharplang/blob/cd938182639870e8f8fa7c7a9cf0542a1eeeac5b/proposals/unsafe-evolution.md#fields
Microsoft Reviewers: Open in CodeFlow